Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove panic from C invalid-argument callbacks #288

Closed

Conversation

apoelstra
Copy link
Member

The upstream C functions will return false after calling these callbacks, at which point we will (usually) panic in the Rust code.

We are not allowed to panic from the callbacks themselves, since the result would be a panic crossing FFI boundaries, which is UB (and results in segfaults in Rust nightly as of 2021-03-11).

@elichai
Copy link
Member

elichai commented Mar 12, 2021

There'e already previous discussion on this subject on: #228
I think the reason for the segfault is the merging of this PR: rust-lang/rust#76570.
I need to read the C-unwind carefully again, as I remember it said that unwinding through extern "C" should abort the process.

@real-or-random
Copy link
Collaborator

real-or-random commented Mar 13, 2021

I don't understand this. Doesn't this say that https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md#abi-boundaries-and-unforced-unwinding that calling panic!() even from "C" is never UB?

edit: I mean of course only in the recent versions that implement this but that's where we see the segfault. Maybe because the RFC is not yet fully implemented? rust-lang/rust#74990

@elichai
Copy link
Member

elichai commented Mar 13, 2021

I don't understand this. Doesn't this say that https://github.com/rust-lang/rfcs/blob/master/text/2945-c-unwind-abi.md#abi-boundaries-and-unforced-unwinding that calling panic!() even from "C" is never UB?

edit: I mean of course only in the recent versions that implement this but that's where we see the segfault. Maybe because the RFC is not yet fully implemented? rust-lang/rust#74990

It looks like it doesn't segfault, it SIGILL, which is apparently is used by LLVM to abort:
rust-lang/rust#52633

So the nightly is doing exactly what the RFC says

@real-or-random
Copy link
Collaborator

real-or-random commented Mar 13, 2021

Ah okay! Maybe we should simply call https://doc.rust-lang.org/std/process/fn.abort.html . I wasn't aware of this function. The process "crashes" cleanly and without UB. Isn't that exactly what we want?

edit: ah this was metnioned in #228, and it requires std of course... rust-lang/rfcs#2512 (comment)

@real-or-random
Copy link
Collaborator

Ok, better idea: Can't we write the callback in C and call __builtin_trap()? This even works on wasm:

https://github.com/WebAssembly/wasi-libc/blob/5ccfab77b097a5d0184f91184952158aa5904c8d/libc-bottom-half/sources/abort.c

@apoelstra
Copy link
Member Author

We cannot use std::process::abort because it's not available with nostd.

This __builtin_trap solution seems pretty nice though!

@apoelstra
Copy link
Member Author

There's also an argument that we could just leave the existing panic in place and just remove the unit test, since we cannot test a SIGILL.

@real-or-random
Copy link
Collaborator

Ok so I think two by far best solutions for us are:

  • Stick with panic!(). For Rust versions that implement RFC 2945, this is exactly what we want (with the minor caveat that the SIGILL is not elegant). For older versions we have technically UB, but we've never seen a compiler exploiting this in bad ways, and future versions will implement RFC 2945.
  • Write the handler in C and use __builtin_trap. This is never UB. The minor caveat is that this requires gcc or clang. As far as I understand, cc could use MSVC in some of our builds?! If yes, that's not nice anyway (for performance) but we could implement an alternative abort for MSVC. As long as we don't need to specialize this for 20 different compilers, this is doable.

@apoelstra
Copy link
Member Author

I think we should just leave the panic in place.

@elichai
Copy link
Member

elichai commented Apr 7, 2021

I think this can be closed now that #290 is merged? (it is technically still UB on stable rust, but
A. this path should never take in production sound code.
B. in practice it currently works correctly because there's a lot of code out there that relies on that behavior (which is the motivation for the c-unwind RFC))

@real-or-random
Copy link
Collaborator

Yeah, I think this can be closed for now. I heard that the change in behavior may be reverted soon :( . But then we can still reconsider this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants